-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optional single-threaded feature to bevy_ecs/bevy_tasks #6690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I'm sold on the feature based approach. It makes this relatively maintainable, and very easy to toggle on and off at a high-level.
There is some open space for discussion on whether we want it to be a feature that is turned on or off to go single threaded. |
Conceptually it feels like "multithreading" is a feature we should enable, which should be turned on by default. We should make sure that both single-threaded and multithreaded paths are being checked in CI (and maybe in miri even): there's a lot of room for subtle mistakes to be introduced. |
Could this also allow us to make bevy_tasks an optional dependency for bevy_ecs? The task pools are only used in a few places. Specifically, the par_for_each methods on World (which would be reduced to shims on serial for_each), the parallel executor (which could be #[cfg] disabled entirely) and in benches (which could keep bevy_tasks as a dev-dependency). |
We could. However, this would mean that types like ParallelExeuctor straight up don't exist when single threaded is disabled, which can cause compile failures when the feature is enabled. Other parts of the engine will still rely on it since we still need an async executor for async tasks (i.e. asset loading, maybe networking). |
Other parts of the engine rely on bevy_tasks or on the parallel executor? If it's the former, could they just pull the parts of bevy_tasks they need out of their own dependency on it? |
This looks in flight but I would note the current version has some sections having a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the right approach to me!
Will re-enable once the checks pass. |
You added a new feature but didn't update the readme. Please run |
1 similar comment
You added a new feature but didn't update the readme. Please run |
I want to review this but the diff looks very messed up. It appears to have incorporated all of the PRs I merged today in the merge train :( Can you fix / remake this? |
53985ea
to
af29efe
Compare
You added a new feature but didn't update the readme. Please run |
You added a new feature but didn't update the readme. Please run |
You added a new feature but didn't update the readme. Please run |
You added a new feature but didn't update the readme. Please run |
Objective
Fixes #6689.
Solution
Add
single-threaded
as an optional non-default feature tobevy_ecs
andbevy_tasks
that:ParallelExecutor
as a default runnerTaskPool
QueryParIter::for_each
calls withQuery::for_each
.Removed the
Mutex
andArc
usage in the single-threaded task pool.Future Work/TODO
Create type aliases for
Mutex
,Arc
that change to single-threaaded equivalents where possible.Changelog
Added: Optional default feature
multi-theaded
to that enables multithreaded parallelism in the engine. Disabling it disables all multithreading in exchange for higher single threaded performance. Does nothing on WASM targets.